Improve naming and allow renaming of memory view instances #69
Conversation
* Introduces a heading in the memory inspector view showing a title * The title is initialized by the webview-main with an index * This title is sent via notification to the webview from the webview-main * On hover, an edit button next to the title appears * Editing can be initiated by clicking the button or double-clicking the title * Once edited, the new title is stored in the OptionWidget state * The OptionWidget stores the previous title to support canceling the editing * As the initial title is set via props, OptionWidget updates the title on prop update Fixes eclipse-cdt-cloud#61
There was a problem hiding this comment.
It feels odd that the tab title and the options widget title are initially out of synch:
Ended up on a roundabout path to figuring out why it's probably happening, so the main feedback is to fix the synchronization of message handling.
Perhaps we could include the title in the initial configuration - that would save a request. The listener on the webview side is probably where it needs to be, but it would be nice to avoid prop-drilling if we can - I think we can at least avoid muddling state and props in the OptionsWidget.
src/plugin/memory-webview-main.ts
Outdated
| protected setTitle(webviewParticipant: WebviewIdMessageParticipant, title: string): void { | ||
| this.messenger.sendNotification(setTitleType, webviewParticipant, title); | ||
| } |
There was a problem hiding this comment.
The machinery to set title from plugin -> webview feels a little heavy: at the moment, it's only used on panel initialization (and may not be working correctly, since in my test, the panel showed the default Memory rather than Memory + Index).
On the other hand I don't actually have a suggestion here - see my next comment.
There was a problem hiding this comment.
Right, it indeed is a bit heavy-weight. But I'm not aware of a mechanism to store state across webview instances other than in the extension "main". However, as you've suggested, I've now put updating the title into the setMemoryViewSettingsType message.
| @@ -23,6 +23,7 @@ import { OptionsWidget } from './options-widget'; | |||
|
|
|||
| interface MemoryWidgetProps extends MemoryDisplayConfiguration { | |||
| memory?: Memory; | |||
| initialTitle: string; | |||
There was a problem hiding this comment.
This name is a little confusing - it's really just title, since we don't distinguish between initialTitle and later titles.
It might be possible to avoid prop drilling here and set up the logic for handling the title in the OptionsWidget, since that's the only place it's relevant, but see my next comment for a caveat.
There was a problem hiding this comment.
I've renamed initialTitle into title according to your suggestion, which makes a lot of sense especially now when we avoid the need for storing the title as a state in the OptionsWidget. 👍
Regarding the prop drilling... I didn't find a great approach to avoid that though. I personally have a slight preference for keeping the handling of messenger events in a central place on both ends (i.e. the App for the webview part) to avoid having handlers of messages from the "outside" scattered throughout all react components, but this is just a personal preference.
One way of avoiding prop drilling might be to pull up the header into its own component and import it in the MemoryWidget directly, or even removing the middle layer MemoryWidget altogether. Grouping prop-values into interfaces may also help to achieve better clarity against an exploding number of props. However, I feel this might be something to attack in a separate PR, wdyt?
src/plugin/memory-webview-main.ts
Outdated
| this.setTitle(webviewParticipant, panel.title); | ||
| this.setInitialSettings(webviewParticipant); |
There was a problem hiding this comment.
I believe that both of these lines should be moved into the handler of the readyType notification handler in setWebviewMessageListener. Otherwise, there's no guarantee that the code that sets up the listeners to handle these requests will have run inside the webview - I suspect that that's why I'm seeing just Memory in the webview I created rather than the intended title. This also suggests that we should probably keep all of the message handling centralized in the App class, even if it's a bit inconvenient: that at least provides the guarantee that the messaging is synchronized.
There was a problem hiding this comment.
Excellent suggestion! Done! 👍
| value={this.state.title} | ||
| onChange={this.handleTitleEdit} |
There was a problem hiding this comment.
It might be possible to simplify the state handling here (and avoid storing previousTitle) by not storing the state of the input as title and handling its changes. Instead, set the defaultValue to the current title, let the user do what they want, and then only enact the state change in the title field in the confirmEditedTitle method using the ref for the text input.
Then title would always be passed in as props, and the only job of this widget would be calling the updateTitle method when we're sure the user wants a new title.
There was a problem hiding this comment.
Excellent suggestion! Done! 👍
|
@colin-grant-work Thank you very much for your thorough review and your excellent suggestions. I tried to address all your comments. In particular, we now include the title in the initial configuration to save a request and avoid keeping the |
What it does
Fixes #61
How to test
Review checklist
Reminder for reviewers